Skip to content

[lua] support iOS triplets#16107

Merged
strega-nil merged 14 commits intomicrosoft:masterfrom
luncliff:ios/lua
Apr 20, 2021
Merged

[lua] support iOS triplets#16107
strega-nil merged 14 commits intomicrosoft:masterfrom
luncliff:ios/lua

Conversation

@luncliff
Copy link
Contributor

@luncliff luncliff commented Feb 8, 2021 •
edited
Loading

What does your PR fix?

  • create a new patch with macro __APPLE__ and TARGET_OS_OSX

    • iOS triplets' target platform prohibits system() function

  • Separate lua[tools] for Lua interpreter/compiler

    • This may surprise port users who expect luai/luac as a default output
    • Given lua[tools], when VCPKG_TARGET_IS_IOS, then FATAL_ERROR

  • The CONTROL file is converted to vcpkg.json

  • Removed duplicated vcpkg_configure_cmake and vcpkg_install_cmake when lua[cpp] used

    • In my opinion such behavior is confusing for users. (They might try to link both liblua.a and liblua-c++.a together.)
      Since lua-c++ uses std::exception instead of setjmp, they should be separated unless the user knows the linkage result exactly.

Which triplets are supported/not supported?

Support 4 ios triplets

  • arm64-ios
  • arm-ios
  • x64-ios
  • x86-ios

Does your PR follow the maintainer guide?

Yes

@luncliff
[lua] support iOS triplets
b0cfe41 
* separate interpreter/compiler to lua[tools] feature
* update git-tree SHA
@JackBoosY JackBoosY added category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Feb 8, 2021
ras0219-msft
ras0219-msft requested changes Feb 8, 2021
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

The change to lua[cpp] is not permissible because it makes the feature non-additive -- if a user enables [cpp], this change would make the package no longer provide the C symbols. This violates the maintainer guidelines: https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#do-not-use-features-to-implement-alternatives

Currently, the reason why having liblua.a and liblua-c++.a both available and linkable is that the symbols do not conflict. Users can link both just fine; the "linkage result" is controlled by whether they've requested C symbols via extern "C" or not.

If the changes to lua[cpp] are reverted, this LGTM.

@luncliff
Copy link
Contributor Author

luncliff commented Feb 9, 2021

Thanks for the point, @ras0219-msft !
I will revert the part in next commit. :)

JackBoosY
JackBoosY suggested changes Feb 9, 2021
View reviewed changes
@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 9, 2021

Waiting for merge #16138.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Feb 9, 2021
@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 9, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 9, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed depends:different-pr This PR or Issue depends on a PR which has been filed labels Feb 10, 2021
ras0219-msft
ras0219-msft requested changes Feb 11, 2021
View reviewed changes
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes building tools to be off-by-default, however that matches the general expected behavior of vcpkg packages.

@luncliff
[lua] make 'tools' default-feature
7d53833 
* set `ENABLE_LUA_CPP` for cmake wrapper
@luncliff
Copy link
Contributor Author

luncliff commented Feb 11, 2021

This PR changes building tools to be off-by-default, however that matches the general expected behavior of vcpkg packages.

I was working on multiple PR and forgot about that... 7d53833 added the default-feature for lua[tools].

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Feb 13, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 13, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@JackBoosY
Copy link
Contributor

JackBoosY commented Feb 18, 2021

@ras0219-msft Should be good now.

@luncliff
Copy link
Contributor Author

luncliff commented Mar 29, 2021

Fixed collision after #16603. #15921 can collide with this PR, either.

@luncliff
Copy link
Contributor Author

luncliff commented Mar 30, 2021

Found #16943. Commenting here to remind me to rebase this. :)

ras0219-msft
ras0219-msft requested changes Apr 2, 2021
View reviewed changes
luncliff and others added 3 commits April 3, 2021 23:19
@luncliff @ras0219-msft
Update ports/lua/portfile.cmake
7fa23a2 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@luncliff @ras0219-msft
Update ports/lua/portfile.cmake
904f127 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@luncliff
[lua] import TargetConditionals for apple platform
487a263
@JackBoosY JackBoosY added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Apr 3, 2021
@JackBoosY
Copy link
Contributor

JackBoosY commented Apr 15, 2021

Any progress here?

@luncliff
Copy link
Contributor Author

luncliff commented Apr 16, 2021

Any progress here?

Sorry. I couldn't work on this recently. I will update in this weekend

@JackBoosY JackBoosY marked this pull request as draft April 16, 2021 06:52
luncliff and others added 4 commits April 17, 2021 18:10
@luncliff
Merge branch 'master' into ios/lua
52ef8bc
@luncliff @ras0219-msft
Update ports/lua/vcpkg.json
1a8ee14 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@luncliff @ras0219-msft
Update ports/lua/portfile.cmake
a96600a 
Co-authored-by: Robert Schumacher <roschuma@microsoft.com>
@luncliff
[lua] remove negations for 'tool' config
e6b7151 
use FEATURES instread of INVERTED_FEATURES to prevent confusions
@luncliff luncliff marked this pull request as ready for review April 17, 2021 09:30
@JackBoosY JackBoosY added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Apr 18, 2021
@strega-nil strega-nil merged commit 1045e88 into microsoft:master Apr 20, 2021
@strega-nil
Copy link
Contributor

strega-nil commented Apr 20, 2021

Thanks again @luncliff :)

@luncliff luncliff deleted the ios/lua branch April 20, 2021 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ras0219-msft ras0219-msft ras0219-msft requested changes

+1 more reviewer

@JackBoosY JackBoosY JackBoosY approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@JackBoosY JackBoosY

Labels

category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@luncliff @JackBoosY @ras0219-msft @strega-nil